Fix: Update RST File Based Tests#564
Conversation
MaximilianSoerenPollak
requested review from
AlexanderLanin,
dcalavrezo-qorix and
nradakovic
as code owners
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
MaximilianSoerenPollak
changed the title
MaximilianSoerenPollak
changed the title
|
AI-assisted review (Claude): Critical: 0 | Important: 1 | Suggestions: 3 Overall this is a clear improvement: the new Important
Suggestions
|
|
@AlexanderLanin I in cooperated some of the Review comments, have another look if you can. |
| to be emitted and checked for 2 lines underneath | ||
|
|
||
| There is multiple things that are not allowed for example, you need to have | ||
| a new line between the EXPECT/-NOT and the need that it refers to |
There was a problem hiding this comment.
There needs to be a new line between the EXPECT/-NOT and the need.
If that is not the case, then our parser sees it correctly (as we just filter for '.. ' and '::') but sphinx-needs or more precisely docutils thinks it is normal text and doesn't actually generate the need itself.
Therefore we have a miss match of what we see and what sphinx sees. This is not fixable on our side (making the need when sphinx doesn't see it). So we have to enforce (lightly) that there is a new line.
| specified after the EXPECT/EXPECT-NOT statement. | ||
|
|
||
| This message needs a '[+x]'offset after the 'EXPECT/-NOT' that should point to the need | ||
| that should (not) emit the warning. |
There was a problem hiding this comment.
It seems to practically always be [+2]. So it feels more like unnecessary boilerplate.
There was a problem hiding this comment.
That is currently cause we only have 1 EXPECT there. If we had 2 or more it would be `
EXPECT[+3]: ...
EXPECT[+2]: ...
etc.
It truly is only an offset that EXACTLY tells us where they want the warning to be.
This way we can also safeguard a bit against sphinx not seeing a need but we do.
Delete unused lines Clear up why some things are done Adapt If check for empty file
MaximilianSoerenPollak
force-pushed
the
MSP_update_rst_tests
branch
from
1c009ca to
d6dbf9e
Compare
|
|
||
| RST_FILES = [str(f.relative_to(RST_DIR)) for f in Path(RST_DIR).rglob("*.rst")] | ||
|
|
||
| logger = get_logger(__file__) |
There was a problem hiding this comment.
AI-assisted review (Claude):
logger is imported and instantiated here but never called anywhere in this file. Dead code — drop both lines.
| logger = get_logger(__file__) |
AlexanderLanin
left a comment
AlexanderLanin
left a comment
There was a problem hiding this comment.
AI-assisted review (Claude):
Critical: 0 | Important: 0 | Suggestions: 1
Good shape. The previous round's concerns are all addressed:
- Dead
DOCS_DIRremoved ✅ if not rst_data_rawdead guard removed ✅- Missing space in the offset-1 error message fixed ✅
- Commented-out debug print syntax error fixed ✅
- Warning filter broadening: the comment on line 302–304 explains the rationale. Acceptable. ✅
monkeypatch.chdirreplacingos.chdir✅
One small thing remaining: logger (lines 22 + 29) is imported and instantiated but never called. See inline comment.
github-project-automation
Bot
moved this from Backlog
to Done
in INF - Infrastructure Community
Added tests in eclipse-score#558 and in parallel modified tests in eclipse-score#564. That created a conflict which only became obvious once both were merged to main.
3 participants
📌 Description
Current RST tests were quite hard to code directly and were error prone without having any insight.
This PR changes the RST tests to make them more explicit as well as making future expandability easier possible
🚨 Impact Analysis
✅ Checklist